Add a smoke test after deployment #69
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the Azure deployment workflow by adding automated smoke tests to validate the deployed service. The changes include refactoring the OpenAI client configuration for better maintainability and implementing a comprehensive testing pipeline.
- Refactored OpenAI client configuration with cleaner imports and improved error handling
- Added comprehensive smoke tests including basic HTTP checks and end-to-end Playwright tests
- Updated dependency specification for OpenAI package version pinning
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/requirements.txt | Pins OpenAI version to exact match for dependency stability |
| src/quartapp/chat.py | Refactors OpenAI client configuration with cleaner imports and improved logging |
| scripts/e2e_chat_playwright.py | Adds new end-to-end browser test using Playwright for chat functionality validation |
| .github/workflows/azure-dev.yaml | Extends workflow with smoke test job including basic HTTP checks and Playwright tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.github/workflows/azure-dev.yaml
Outdated
| build: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| uri: ${{ steps.output.outputs.uri }} |
There was a problem hiding this comment.
Inconsistent indentation: the uri output should be indented with 6 spaces to align with the outputs: key, not 8 spaces.
| uri: ${{ steps.output.outputs.uri }} | |
| uri: ${{ steps.output.outputs.uri }} |
| ) | ||
| current_app.logger.info("Using Azure OpenAI with key") | ||
| elif os.getenv("RUNNING_IN_PRODUCTION"): | ||
| client_id = os.environ["AZURE_CLIENT_ID"] |
There was a problem hiding this comment.
Using os.environ directly will raise a KeyError if the environment variable is missing. Consider using os.getenv() with a descriptive error message or add proper error handling to provide clearer feedback when the required environment variable is not set.
| client_id = os.environ["AZURE_CLIENT_ID"] | |
| client_id = os.getenv("AZURE_CLIENT_ID") | |
| if not client_id: | |
| raise RuntimeError("AZURE_CLIENT_ID environment variable is not set but is required for managed identity authentication.") |
| base_url=os.environ["AZURE_OPENAI_ENDPOINT"] + "/openai/v1/", | ||
| api_key=token_provider, | ||
| ) | ||
| tenant_id = os.environ["AZURE_TENANT_ID"] |
There was a problem hiding this comment.
Using os.environ directly will raise a KeyError if the environment variable is missing. Consider using os.getenv() with a descriptive error message or add proper error handling to provide clearer feedback when the required environment variable is not set.
| tenant_id = os.environ["AZURE_TENANT_ID"] | |
| tenant_id = os.getenv("AZURE_TENANT_ID") | |
| if tenant_id is None: | |
| raise RuntimeError("AZURE_TENANT_ID environment variable is not set. Please set it to continue.") |
Purpose
This pull request updates the Azure deployment workflow with an automated smoke test to validate the deployed chat endpoint. The "Smoke Test Chat Endpoint" step automatically checks the
/chat/streamendpoint of the deployed service, retrying up to 6 times to confirm it responds with HTTP 200, improving early detection of deployment issues.Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test